feat: base impl for ecs fargate cdk #371
Conversation
![datadog-datadog-prod-us1[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
| * Enables Dogstatsd traffic over Unix Domain Socket. | ||
| * Falls back to UDP configuration for application containers when disabled | ||
| */ | ||
| readonly enableDogstatsdSocket?: boolean; |
There was a problem hiding this comment.
⚪ Code Quality Violation
Prop name (enableDogstatsdSocket) doesn't match rule (is|has) (...read more)
Enforces a consistent naming pattern for boolean props.
The pattern is: "^(is|has)[A-Z]([A-Za-z0-9]?)+" to enforce is and has prefixes.
There was a problem hiding this comment.
Will look into prefixing these bools with "is"
Might also change the configuration for each feature to have their own sub-interfaces too.
|
|
||
| // CWS Configuration: | ||
| if (props.enableCWS) { | ||
| const containerProps = (container as any).props as ecs.ContainerDefinitionProps; |
There was a problem hiding this comment.
🟠 Code Quality Violation
Unsafe assignment of an `any` value. (...read more)
The any type in TypeScript is dangerously broad, leading to unexpected behavior. Using any should be avoided.
|
|
||
| const currentEntryPoint = containerProps.entryPoint as string[]; | ||
| currentEntryPoint.unshift(...entryPointPrefixCWS); | ||
| (container as any).props.entryPoint = currentEntryPoint; |
There was a problem hiding this comment.
🟠 Code Quality Violation
Unsafe assignment of an `any` value. (...read more)
The any type in TypeScript is dangerously broad, leading to unexpected behavior. Using any should be avoided.
There was a problem hiding this comment.
Looking for opinions on this. I need to access a few values in the container such as linuxParameters, entryPoint, etc. to properly configure Datadog. However, many of these are private and/or readonly.
I've tried getting the cfn definition of the task/container when possible to retrieve the values more safely, but some seem to be just impossible to do safely and requires the (as any).
| (container as any).props.entryPoint = currentEntryPoint; | ||
|
|
||
| if (container.linuxParameters === undefined) { | ||
| (container as any).linuxParameters = new ecs.LinuxParameters(this.scope, "LinuxParameters", {}); |
There was a problem hiding this comment.
🟠 Code Quality Violation
Unsafe assignment of an `any` value. (...read more)
The any type in TypeScript is dangerously broad, leading to unexpected behavior. Using any should be avoided.
There was a problem hiding this comment.
A suggestion based on my preference: For future PRs, is it possible to keep each PR small, maybe no more than 100 lines of code (not including tests)? Thus it's more clear what each PR does, makes it more clear which test is for which code, making review easier. Examples: https://github.com/DataDog/datadog-cloudformation-macro/pulls?q=is%3Apr+is%3Aclosed+%5BStep+Function%5D
src/index.ts
Outdated
| export * from "./ecs/interfaces"; | ||
| export * from "./ecs/constants"; | ||
| export * from "./ecs/fargate/interfaces"; | ||
| export * from "./ecs/environment"; | ||
| export * from "./ecs/utils"; |
There was a problem hiding this comment.
Can we delete these lines? I think ideally we should only export these files:
export * from "./datadog-lambda";
export * from "./datadog-step-functions";
export * from "./ecs/fargate/datadog-ecs-fargate";
and in these files, only expose those functions that are intended to be external API functions.
There was a problem hiding this comment.
Yes I agree we should only expose the Construct + Props externally. I can make edits to support this.
|
Thanks for creating the |
Sorry I got a bit carried away after having an MVP and started adding features like APM, DSD, Logs to test it out. In the future they will definitely be smaller! |
gabedos
changed the title
What does this PR do?
This creates the base implementation for the Datadog ECS Fargate CDK. This includes features such as:
Motivation
Allow ECS Fargate customers to easily instrument their applications to properly run Datadog features.
Testing Guidelines
Tests TBD on general feedback. Otherwise tests will be created for each and every method to ensure they properly augment the container definitions properly.
Additional Notes
Types of Changes
Check all that apply